Skip to content

Conversation

@yeya24
Copy link
Contributor

@yeya24 yeya24 commented Dec 10, 2025

What this PR does:

This PR supports projection pushdown in Parquet Queryable. New config is added to honor projection hints.

Projection pushdown is only enabled if:

  • Thanos engine is enabled and projection optimizer is enabled
  • The query request can only be used served by parquet blocks and it shouldn't fallback to store gateway
  • The query shouldn't query ingesters as ingester doesn't support pushdown today

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@SungJin1212
Copy link
Member

LGTM, can you check the lint CI?

@yeya24
Copy link
Contributor Author

yeya24 commented Dec 11, 2025

@SungJin1212 I am waiting for prometheus-community/parquet-common#128 to be merged first. The current projection hints implementation will fail for the default case where the query cannot use projection pushdown

@yeya24 yeya24 force-pushed the expose-projection-hints branch 3 times, most recently from 5b2e6a0 to e25a78e Compare December 12, 2025 06:17
@SungJin1212
Copy link
Member

LGTM, can you add a changelog?

Copy link
Member

@SungJin1212 SungJin1212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 16, 2025
@yeya24 yeya24 force-pushed the expose-projection-hints branch from 68d3a4d to efd096d Compare December 17, 2025 07:25
Signed-off-by: yeya24 <benye@amazon.com>
@yeya24 yeya24 force-pushed the expose-projection-hints branch from efd096d to 4552f4a Compare December 17, 2025 07:28
Copy link
Member

@alanprot alanprot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a small question.

sortSeries = true
}

queryIngesters := q.queryIngestersWithin == 0 || maxt >= util.TimeToMillis(time.Now().Add(-q.queryIngestersWithin).Add(-q.projectionHintsIngesterBuffer))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This maxt is the original maxt or is the maxt already changed to not contain the timestamp for data in the ingesters?

blocksStoreQuerier storage.Querier

// If set, the querier manipulates the max time to not be greater than
// "now - queryStoreAfter" so that most recent blocks are not queried.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what worried about.. that the maxT is already manipulated so we do not see that the query was going to ingesters?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component/querier lgtm This PR has been approved by a maintainer size/XL type/feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants